The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Claude finished @alpe's task in 2m 3s —— View job

Code Review Complete

I've performed a comprehensive code review of this P2P client reuse and pending block storage refactoring PR. Below are my findings organized by category.

1. Architecture & Design Quality ⚠️

Strengths:

• Centralized P2P lifecycle management: Moving P2P client creation to the top-level node constructors is a solid architectural improvement that eliminates redundant initialization across mode switches
• Clean dependency injection: The refactoring properly passes *p2p.Client down through constructors, maintaining clear ownership
• Concurrent sync startup: The failover.go:192-208 parallel initialization of header and data sync services reduces cumulative startup delays

Issues:

CRITICAL - Missing P2P Client Reconfiguration (node/failover.go:104-163)
The PR description and Gemini's review mention that Reconfigure() is called to update P2P configuration during mode switches, but this call does not exist in setupFailoverState. The P2P client is passed through but never reconfigured when transitioning between aggregator and sync modes. This could lead to:

• Stale P2P configuration after mode switches
• Incorrect PubSub topics or DHT parameters for the new mode
• Resource leaks if certain P2P resources aren't updated

Recommendation: Either:

1. Add the missing p2pClient.Reconfigure(nodeConfig.P2P) call in setupFailoverState, OR
2. Update documentation/comments to clarify that reconfiguration is not needed (with justification)

Reference: node/failover.go:104-163

MEDIUM - P2P Client Start/Stop Lifecycle Issue (node/full.go:284-292)
The P2P client is started once in FullNode.Run() and closed in the defer, but the failoverState previously managed its own P2P lifecycle. This change means:

• If leaderElection.Run() fails early, the P2P client will still be running until the defer cleanup
• There's a potential race between mode switches and P2P operations if the client is shared across concurrent mode transitions
• The error handling at line 289-291 logs errors but doesn't propagate them

Recommendation:

• Consider whether P2P client errors during close should be propagated or just logged
• Document the thread-safety guarantees of the shared P2P client across mode switches

2. Concurrency & Resource Management ⚠️

Issues:

MEDIUM - Concurrent Sync Service Startup Error Handling (node/failover.go:192-208)
The concurrent startup of sync services uses errgroup, which is good, but:

syncWg, syncCtx := errgroup.WithContext(ctx)
syncWg.Go(func() error {
    if err := f.headerSyncService.Start(syncCtx); err != nil {
        return fmt.Errorf("header sync service: %w", err)
    }
    return nil
})

Problems:

• If one service fails during startup, the other continues running but may not be properly stopped
• The deferred stopService calls at lines 209-210 might try to stop a service that never fully started
• The order of cleanup (defer statements) means data sync stops before header sync, but they started concurrently

Recommendation:

• Add explicit cleanup for partially-started services when syncWg.Wait() returns an error
• Document the expected behavior when one sync service fails during concurrent startup

LOW - P2P Client Thread Safety (pkg/p2p/client.go:117-174)
The started field (line 57) is a simple bool, not atomic. While Start() checks if c.started { return nil }, concurrent calls to Start() could race:

func (c *Client) Start(ctx context.Context) error {
    if c.started {  // ← Race condition
        return nil
    }
    // ... initialization
    c.started = true
}

Recommendation: Use atomic.Bool or add mutex protection to prevent potential double-initialization if Start() is called concurrently.

3. Error Handling & Resilience ✅

Strengths:

• Proper use of errors.Join for aggregating multiple shutdown errors (failover.go:172, full.go:314)
• Timeout contexts for shutdown operations (3s in failover, 9s in full node)
• Retry logic in sync service P2P initialization with exponential backoff (pkg/sync/sync_service.go:383-413)

Minor Issues:

LOW - Inconsistent Error Context (pkg/cmd/run_node.go:167-170)
When P2P client creation fails, the error message is generic:

p2pClient, err := p2p.NewClient(...)
if err != nil {
    return fmt.Errorf("create p2p client: %w", err)
}

This doesn't indicate where in the startup process this occurred or what the implications are.

Recommendation: Add more context, e.g., "failed to create P2P client for node initialization: %w"

4. Testing & Reliability ⚠️

Coverage Concerns:

CRITICAL - Test Coverage is 40.57% (Codecov Report)
The PR has 82 lines missing coverage, with major gaps in:

• block/internal/executing/pending.go: 40 missing, 12 partial
• block/internal/executing/executor.go: 2 missing, 4 partial
• node/failover.go: 3 missing, 3 partial
• pkg/cmd/run_node.go: 4 missing (0% coverage)

Recommendation: Add tests for:

1. P2P client reuse across multiple node restarts
2. Failover between aggregator and sync modes with shared P2P client
3. Concurrent sync service startup failure scenarios
4. The new concurrent initialization path in failover.go

Test Quality Issues:

MEDIUM - Test Helper Inconsistency (node/helpers_test.go:71-83)
The createTestComponents function now returns crypto.PrivKey directly, but the function signature comment wasn't updated. Also, the returned private key is always the genesis validator key, which might not be appropriate for multi-node tests with different identities.

Recommendation: Document that the same key is reused and consider whether test isolation requires unique keys.

LOW - Missing Test for P2P Interface (pkg/sync/sync_service.go:40-47)
The new P2PClient interface is good abstraction, but there's no test verifying that *p2p.Client actually implements this interface. If the P2P client API changes, this could break at runtime.

Recommendation: Add a compile-time interface check:

var _ P2PClient = (*p2p.Client)(nil)

5. Security Considerations ✅

Strengths:

• Private keys are properly passed through as crypto.PrivKey without exposure
• No hard-coded credentials or secrets
• Proper separation of concerns between P2P identity and application logic

Minor Notes:

• The P2P client stores the private key (pkg/p2p/client.go:82) but only exposes public methods
• Connection gater properly manages peer blocking/allowing (client.go:74-76)

6. Performance Considerations ⚡

Improvements:

• Concurrent sync service startup reduces initialization time - Good optimization for nodes syncing from peers
• P2P client reuse eliminates redundant libp2p initialization - Significant improvement for HA scenarios with frequent mode switches

Potential Issues:

MEDIUM - Store Adapter Pending Cache Growth (pkg/store/store_adapter.go:24, 614-677)
The pending cache has a hard limit of 10,000 items with backpressure blocking:

const maxPendingCacheSize = 10_000
// ...
func (a *StoreAdapter[H]) waitForSpace(ctx context.Context) error {
    for {
        if a.pending.len() < maxPendingCacheSize {
            return nil
        }
        // ... prune and retry
    }
}

Problems:

• If executor falls significantly behind P2P gossip, this will block incoming P2P messages
• The pruneRetryInterval of 50ms could cause head-of-line blocking in the P2P layer
• No metrics/logging when backpressure is applied

Recommendation:

• Add logging/metrics when cache reaches capacity
• Consider dynamic backpressure based on executor lag
• Document the expected behavior under high load

7. Documentation & Code Quality ⚠️

Issues:

MEDIUM - Misleading Comment (node/failover.go:104-163)
As mentioned in the Gemini review, there's no Reconfigure() call in setupFailoverState despite documentation suggesting it exists. This is confusing for maintainers.

LOW - Inconsistent Import Style (multiple files)
Some files use:

import (
    "github.com/evstack/ev-node/pkg/p2p"
)

While others use:

import (
    p2pkey "github.com/evstack/ev-node/pkg/p2p/key"
)

The removal of the p2p/key package references is correct, but ensure consistency across the codebase.

LOW - Test Comment Clarity (block/components_test.go:100-106)
The comments say "nil arguments" but don't explain why nil is acceptable:

// Create noop DAHintAppenders for testing
headerHintAppender := noopDAHintAppender{}  // ← Not nil
dataHintAppender := noopDAHintAppender{}    // ← Not nil

Recommendation: Update comments to match actual code.

Summary & Recommendations

Must Fix Before Merge:

1. ⛔ Investigate missing p2pClient.Reconfigure() call - Either add it or document why it's not needed
2. ⚠️ Improve test coverage - 40.57% is too low for core infrastructure changes
3. ⚠️ Add P2P client thread-safety protection - Use atomic.Bool for started field

Should Fix:

4. Add explicit cleanup for partial sync service startup failures
5. Add logging/metrics for pending cache backpressure
6. Fix misleading documentation about Reconfiguration
7. Add tests for P2P client reuse across restarts

Nice to Have:

8. Improve error messages with more context
9. Add compile-time interface checks for P2PClient
10. Document thread-safety guarantees for shared P2P client

Overall Assessment

This is a solid architectural improvement that addresses real performance issues with P2P client initialization. The core design is sound, but there are several gaps in error handling, testing, and documentation that should be addressed before merging. The missing reconfiguration logic is particularly concerning and needs clarification.

Recommendation: Request changes focused on the critical issues, particularly the P2P reconfiguration question and test coverage improvements.

Summary of Changes

Hello @alpe, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the P2P client's lifecycle management within the node architecture. By centralizing the P2P client's creation and making it persistent across node mode switches, it aims to improve startup efficiency and reduce resource overhead. Additionally, it introduces concurrent initialization for synchronization services and strengthens data consistency checks in the store getters, ensuring that only fully committed data is exposed via the P2P layer.

Highlights

• Centralized P2P Client Management: The P2P client is now instantiated once at the top-level FullNode or LightNode creation and passed down to sub-components. This ensures the P2P client's lifecycle is managed independently of node mode switches (e.g., between aggregator and sync modes), preventing costly teardown and re-initialization of the libp2p stack.
• Decoupled P2P Client Lifecycle: The p2p.Client now includes a Reconfigure method, allowing its internal configuration to be updated without restarting the underlying libp2p host, PubSub, or DHT. This is crucial for seamless transitions between node operational modes.
• Concurrent Sync Service Startup: The header and data synchronization services in failover.go now start concurrently using errgroup.WithContext. This optimization reduces cumulative startup delays, especially in scenarios where peers might initially lack blocks.
• Enhanced Header and Data Store Guards: New height guards have been implemented in HeaderStoreGetter and DataStoreGetter. These guards ensure that only headers and data at or below the committed store height are returned, preventing the P2P layer from serving unsigned or uncommitted data (e.g., from an executor's early save during crash recovery).
• Refactored Node Constructors: Node constructors (newFullNode, newLightNode, newSyncMode, newAggregatorMode) and related test helpers have been updated to accept an initialized p2p.Client or crypto.PrivKey directly, simplifying their signatures and reinforcing the centralized P2P client management.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Codecov Report

❌ Patch coverage is 51.28205% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.03%. Comparing base (d210055) to head (f854686).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3065      +/-   ##
==========================================
- Coverage   61.07%   61.03%   -0.05%     
==========================================
  Files         113      113              
  Lines       11431    11440       +9     
==========================================
  Hits         6982     6982              
- Misses       3669     3676       +7     
- Partials      780      782       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@claude review again

Claude finished @alpe's task in 2m 21s —— View job

Code Review Complete

I've performed a comprehensive code review of this P2P client refactoring PR. Here are my findings:

Overview

This PR successfully refactors the P2P client lifecycle to be centralized and reused across node mode switches (aggregator ↔ sync). The core architectural improvement is solid, moving P2P client creation from individual mode constructors to the top-level node initialization. However, there are several important issues that should be addressed.

Critical Issues

1. Thread-Safety Race Condition in started Flag

Location: pkg/p2p/client.go:57, pkg/p2p/client.go:125-127

Issue: The started boolean field is not thread-safe and can race if Start() is called concurrently:

func (c *Client) Start(ctx context.Context) error {
    if c.started {  // ← Race: read without synchronization
        return nil
    }
    // ... initialization code ...
    c.started = true  // ← Race: write without synchronization
}

Impact: If Start() is called concurrently (which could happen during rapid mode switches or in tests), the P2P client could be initialized twice, leading to resource leaks (duplicate DHT, multiple PubSub instances, etc.).

Recommendation: Use atomic.Bool instead:

// In Client struct:
started atomic.Bool

// In Start():
if !c.started.CompareAndSwap(false, true) {
    return nil  // Already started
}

2. Missing Test Coverage (51.28% patch coverage)

Location: Multiple files, see Codecov report

Issue: The PR has significant test coverage gaps:

• pkg/cmd/run_node.go: 0% coverage (4 missing lines)
• node/failover.go: 60% coverage (3 missing, 3 partial)
• node/full.go: 50% coverage (2 missing, 2 partial)
• node/light.go: 0% coverage for new code (1 missing, 1 partial)

Missing test scenarios:

1. P2P client reuse across node restarts
2. Concurrent Start() calls on the same P2P client
3. Mode switch from aggregator to sync and back
4. P2P client lifecycle during failover scenarios
5. Concurrent sync service startup failure handling (lines 192-208 in failover.go)

Recommendation: Add integration tests covering:

• Multiple node restarts reusing the same P2P client
• Concurrent mode switches with shared P2P client
• Failure scenarios during concurrent sync service startup

Major Issues

3. Unclear P2P Client Reconfiguration Strategy

Location: node/failover.go:104-163

Issue: Previous reviewers (Gemini) mentioned that Reconfigure() should be called during mode switches to update P2P configuration, but no such call exists in setupFailoverState. Looking at the code:

1. The P2P client is created once in pkg/cmd/run_node.go:167-170
2. It's passed through to both aggregator and sync modes
3. But no reconfiguration happens when switching modes

Questions:

• Does the P2P configuration need to change between aggregator and sync modes?
• Are there different PubSub topics, DHT parameters, or peer lists per mode?
• If not, why was this mentioned in previous reviews?

Current behavior: The P2P client retains the configuration it was initialized with, even after mode switches.

Recommendation: Either:

1. Add p2pClient.Reconfigure(nodeConfig.P2P) in setupFailoverState if reconfiguration is needed, OR
2. Document clearly that P2P configuration is static across mode switches and explain why this is acceptable

4. Concurrent Sync Service Startup - Partial Failure Handling

Location: node/failover.go:192-210

Issue: The concurrent startup of header and data sync services has incomplete error handling:

syncWg, syncCtx := errgroup.WithContext(ctx)
syncWg.Go(func() error {
    if err := f.headerSyncService.Start(syncCtx); err != nil {
        return fmt.Errorf("header sync service: %w", err)
    }
    return nil
})
syncWg.Go(func() error {
    if err := f.dataSyncService.Start(syncCtx); err != nil {
        return fmt.Errorf("data sync service: %w", err)
    }
    return nil
})
if err := syncWg.Wait(); err != nil {
    return err  // ← Returns immediately on error
}
defer stopService(f.headerSyncService.Stop, "header sync")
defer stopService(f.dataSyncService.Stop, "data sync")

Problem: If one service fails during startup:

• The errgroup context is canceled, which may stop the other service
• But we immediately return err before reaching the deferred stopService calls
• This means if headerSyncService.Start() succeeded but dataSyncService.Start() failed, the header sync service keeps running and is never stopped

Recommendation: Add explicit cleanup when syncWg.Wait() returns an error:

if err := syncWg.Wait(); err != nil {
    // Clean up any partially started services
    cleanupCtx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
    defer cancel()
    _ = f.headerSyncService.Stop(cleanupCtx)
    _ = f.dataSyncService.Stop(cleanupCtx)
    return err
}

Medium Priority Issues

5. P2P Client Error Handling in FullNode.Run()

Location: node/full.go:288-292

Issue: When P2P client fails to close during shutdown, the error is only logged but not propagated:

defer func() {
    if err := n.p2pClient.Close(); err != nil {
        n.Logger.Error().Err(err).Msg("error closing p2p client")
        // ← Error is not added to shutdownMultiErr
    }
}()

Impact: P2P shutdown errors are silently swallowed, making it hard to diagnose resource leaks.

Recommendation: Consider whether P2P close errors should be added to shutdownMultiErr for consistency with other shutdown operations.

6. P2PClient Interface Missing Compile-Time Check

Location: pkg/sync/sync_service.go:40-47

Issue: The new P2PClient interface is a good abstraction, but there's no compile-time verification that *p2p.Client implements it. If the P2P client API changes, this could break at runtime.

Recommendation: Add a compile-time interface assertion:

// In pkg/p2p/client.go or pkg/sync/sync_service.go
var _ sync.P2PClient = (*p2p.Client)(nil)

7. Misleading Comment About daStore Usage

Location: node/failover.go:113-121 (in the diff context)

Observation: The previous code used a separate daStore parameter, but now both sync services use rktStore. This is correct based on the refactoring, but ensure this doesn't cause issues if the store implementations differ.

Action: Verify that using rktStore for both header and data sync services is intentional and correct.

Minor Issues

8. Inconsistent Error Context Messages

Location: pkg/cmd/run_node.go:167-170

Issue: The error message when P2P client creation fails is generic:

p2pClient, err := p2p.NewClient(...)
if err != nil {
    return fmt.Errorf("create p2p client: %w", err)
}

Recommendation: Add more context about the stage and implications:

return fmt.Errorf("failed to create P2P client during node initialization: %w", err)

9. Documentation Gap

Location: pkg/p2p/client.go:117-174

Issue: The Start() method now has idempotency (returns nil if already started), but this behavior isn't documented in the method's godoc comment.

Recommendation: Update the godoc:

// Start establishes Client's P2P connectivity. This method is idempotent and
// safe to call multiple times - subsequent calls return nil if already started.
//
// Following steps are taken:
// ...

Positive Aspects

1. Solid architectural improvement: Centralizing P2P client lifecycle is the right approach
2. Resource efficiency: Reusing the P2P client across mode switches avoids costly libp2p re-initialization
3. Concurrent sync startup: Good optimization to reduce cumulative startup delays (lines 192-208)
4. Clean dependency injection: P2P client is properly passed through constructors
5. Error aggregation: Good use of errors.Join for combining shutdown errors

Test Coverage Breakdown

According to Codecov, the patch coverage is 51.28% with 19 lines missing coverage:

Recommendations Summary

Must Fix (Before Merge):

1. ⛔ Fix thread-safety race in started flag - Use atomic.Bool
2. ⚠️ Add explicit cleanup for partial sync service startup failures
3. ⚠️ Clarify P2P reconfiguration strategy - Document or implement

Should Fix:

4. 📊 Improve test coverage - Add integration tests for P2P client reuse and mode switches
5. 🔍 Add compile-time interface check for P2PClient
6. 📝 Document idempotent Start() behavior

Nice to Have:

7. Consider propagating P2P close errors in shutdown
8. Improve error messages with more context
9. Verify rktStore usage is correct for both sync services

Overall Assessment

This is a solid architectural refactoring that addresses real performance and resource management issues. The core design is sound, but there are critical thread-safety and error handling gaps that must be addressed before merging.

Recommendation: Request changes to address the thread-safety issue (#1), partial failure cleanup (#4), and P2P reconfiguration clarity (#3). Test coverage improvements (#4) are also highly recommended.

The concurrent sync service startup optimization is excellent, but needs better error handling. Once the critical issues are addressed, this will be a valuable improvement to the codebase.